editor/app/(www)/(pricing)/pricing/_sections/faq.tsx-56-59 (1)

56-59: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

FAQ describes multi-seat billing mechanics that are explicitly deferred in this PR.

The PR description states: "Single-seat subscriptions only: every subscription is created and maintained with quantity = 1; multi-seat billing deferred (tracked: GRIDA-60)."

Yet this FAQ item tells users that adding a member "adds a seat (prorated for the rest of the period)", removing one "credits the next invoice", and gives a concrete 5-seat pricing example — all of which currently behave differently (quantity stays at 1). Users who rely on this as documentation will be confused and file support tickets when seat-syncing doesn't happen.

Consider either:

• Replacing the forward-looking mechanics with the actual current behaviour ("seats are billed manually for now; multi-seat automation is coming"), or
• Deferring this FAQ item until GRIDA-60 ships.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/app/`(www)/(pricing)/pricing/_sections/faq.tsx around lines 56 - 59,
The FAQ entry under pricing/_sections/faq.tsx that has question "How does
pricing work for teams?" currently describes multi-seat billing mechanics;
update the answer to reflect the current single-seat behavior (every
subscription is created/maintained with quantity = 1 and multi-seat automation
is deferred: reference GRIDA-60) or remove/hold the entire FAQ object until
GRIDA-60 ships; modify the object whose question string equals "How does pricing
work for teams?" so the answer explicitly states seats are billed manually /
multi-seat support is not yet implemented and avoid concrete prorated/5-seat
examples.

editor/app/(site)/organizations/[organization_name]/settings/billing/layout.tsx-4-9 (1)

4-9: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

React is used as a type namespace but never imported — this will cause a TypeScript compilation error.

React.ReactNode references the React namespace at the type level, but unlike JSX syntax (handled transparently by the react-jsx transform), namespace type references still require an explicit import. The project's TypeScript config uses jsx: "react-jsx" which allows JSX without imports, but the compiler will fail on React.ReactNode without the namespace in scope. Compare with editor/app/(embed)/embed/layout.tsx which correctly imports type { ReactNode } from "react" and uses it directly.

🐛 Proposed fix

+import type { ReactNode } from "react";

 export default function BillingLayout({
   children,
   modal,
 }: {
-  children: React.ReactNode;
-  modal: React.ReactNode;
+  children: ReactNode;
+  modal: ReactNode;
 }) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@editor/app/`(site)/organizations/[organization_name]/settings/billing/layout.tsx
around lines 4 - 9, The file declares BillingLayout using React.ReactNode types
but never imports React, causing a TypeScript error; update the top of the file
to import the types (for example: import type { ReactNode } from "react") and
change the prop annotations to use ReactNode (or alternatively import React
itself) so that the BillingLayout props (children, modal) compile correctly;
refer to the BillingLayout function and the React.ReactNode type usage when
making the change.

editor/lib/billing/__tests__/e2e/fixtures/org.ts-80-93 (1)

80-93: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t silently succeed after Stripe cleanup failures.

If subscription cancellation delivery or customer deletion fails here, teardown can still complete locally and leave orphaned Stripe state behind for later runs. For the only cleanup path in these E2E fixtures, that turns transient cleanup failures into hidden account pollution.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/lib/billing/__tests__/e2e/fixtures/org.ts` around lines 80 - 93, The
current teardown swallows Stripe failures (in the subscription cancellation
catch and the stripe.customers.del catch), logging warnings and continuing which
leaves orphaned Stripe state; change both catch blocks to surface failures by
rethrowing the original error (or throw a new Error with the original message)
after logging so the E2E fixture teardown fails instead of silently
succeeding—look for the subscription cancellation try/catch and the
stripe.customers.del call in this file and replace their console.warn-only
catches with code that logs then throws the error.

editor/host/url.ts-52-58 (1)

52-58: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

settings now collides with the existing document-scoped universal route.

There is already a document-scoped settings entry below. Adding another settings path here means the organization route overwrites the document route in UNIVERSAL_ROUTE_CONFIGS, and even a later id-only fix would still leave /_/settings ambiguous. The organization pages need a distinct universal path/id namespace.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/host/url.ts` around lines 52 - 58, ORGANIZATION_ROUTE_CONFIGS
introduces a duplicate "settings" key that collides with the document-scoped
"settings" entry in UNIVERSAL_ROUTE_CONFIGS; rename the org-scoped keys to a
distinct namespace (e.g., "organization/settings" or "org/settings" and
corresponding "organization/settings/billing",
"organization/settings/billing/upgrade") and update any places that merge or
reference ORGANIZATION_ROUTE_CONFIGS so the org routes no longer overwrite the
document-scoped "settings" entry in UNIVERSAL_ROUTE_CONFIGS.

editor/lib/billing/__tests__/e2e/fixtures/deliver-event.ts-75-80 (1)

75-80: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

The non-JSON fallback loses the original response body.

res.json() consumes the stream before throwing on invalid JSON, so the res.text() call in the catch will usually fail too. That makes the helper least useful when the webhook returns an HTML/plaintext error page.

Suggested fix

   let body: DeliverResult["body"];
-  try {
-    body = (await res.json()) as DeliverResult["body"];
-  } catch {
-    body = { error: `non-json: ${await res.text()}` };
-  }
+  const raw = await res.text();
+  try {
+    body = JSON.parse(raw) as DeliverResult["body"];
+  } catch {
+    body = { error: `non-json: ${raw}` };
+  }
   return { status: res.status, body };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/lib/billing/__tests__/e2e/fixtures/deliver-event.ts` around lines 75 -
80, The fallback loses the original response because res.json() consumes the
body and prevents res.text() from reading it; modify the handler in this fixture
so you first read the raw text (e.g., via await res.clone().text() or await
res.text()), store it, then attempt to parse JSON from that raw string and set
body as DeliverResult["body"] accordingly (use the stored raw for the non-JSON
error case). Update the logic around res.json()/res.text() to avoid consuming
the stream twice and ensure the original response body is preserved in the
fallback.

editor/app/(workspace)/universal/[[...path]]/page.tsx-59-62 (1)

59-62: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle the membership query failure instead of rendering the empty state.

If this select errors, memberships becomes null and the branch tells the user they have no organizations. That masks auth/RLS/backend failures as a normal empty result.

Suggested fix

-    const { data: memberships } = await client
+    const { data: memberships, error: membershipsError } = await client
       .from("organization_member")
       .select(`organization:organization(name)`)
       .eq("user_id", auth.user.id);
+    if (membershipsError) throw membershipsError;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/app/`(workspace)/universal/[[...path]]/page.tsx around lines 59 - 62,
The membership query currently destructures only data so query failures (error
!= null) produce memberships === null and are shown as an empty state; update
the call that uses client.from(...).select(...) to also destructure the error
(e.g., const { data: memberships, error } = await
client.from(...).select(...).eq(...)), then branch on error before rendering:
either throw or render an error UI/log the error via your logger instead of
treating null as "no organizations" — make this change where memberships is used
so the component/page (the [[...path]] page) distinguishes real empty results
from backend/auth/RLS failures.

supabase/tests/test_grida_billing_test.sql-282-307 (1)

282-307: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Missing write-isolation coverage on grida_billing tables (per RLS guideline).

Section 9 only proves read isolation on v_billing_subscription for insider/alice/random. The guideline requires both read and write isolation tests (insert/update/delete) for every table that alters RLS or tenant boundaries. Direct writes to grida_billing.subscription, grida_billing.account, and grida_billing.audit by authenticated users (and to v_billing_subscription if that view is updatable) should be asserted to fail — that's what locks the "service_role-only mutation" invariant from the PR description into a regression test.

Suggested additions (sketch):

SET LOCAL ROLE authenticated;
SELECT set_config('request.jwt.claim.sub', current_setting('test.alice_uid'), true);

SELECT throws_ok($$
  INSERT INTO grida_billing.subscription (organization_id, plan, status, is_free, quantity)
  VALUES (2, 'pro', 'active', false, 1)
$$, NULL, NULL, 'authenticated cannot INSERT into grida_billing.subscription');

SELECT throws_ok($$
  UPDATE grida_billing.subscription SET status='canceled' WHERE organization_id=2
$$, NULL, NULL, 'authenticated cannot UPDATE grida_billing.subscription');

SELECT throws_ok($$
  DELETE FROM grida_billing.subscription WHERE organization_id=2
$$, NULL, NULL, 'authenticated cannot DELETE from grida_billing.subscription');

RESET ROLE;

(Also bump plan(37) accordingly.)

As per coding guidelines: "Add pgTAP test coverage for every table that alters RLS, permissions, or tenant boundaries, including at minimum read isolation tests and write isolation tests (insert/update/delete)".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/tests/test_grida_billing_test.sql` around lines 282 - 307, Add
write-isolation pgTAP assertions for grida_billing tables and the
v_billing_subscription view: while running as SET LOCAL ROLE authenticated and
after setting request.jwt.claim.sub (e.g., current_setting('test.alice_uid') and
a random user), assert that INSERT/UPDATE/DELETE on grida_billing.subscription,
grida_billing.account, grida_billing.audit (and v_billing_subscription if
updatable) all throw using throws_ok so authenticated cannot mutate those rows;
include separate throws_ok checks for INSERT, UPDATE, DELETE per table/view and
increment the pgTAP plan number accordingly.

docs/contributing/billing.md-93-93 (1)

93-93: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Link points outside /docs — violates docs guideline.

[test/billing-*.md](../../test/) resolves to /test/, which is outside the docs/ tree. Per the docs guideline, paths outside /docs must be referenced as inline code, not links.

♻️ Proposed fix

-User-facing billing docs: [`docs/platform/billing.mdx`](../platform/billing.mdx). Behaviour test cases: [`test/billing-*.md`](../../test/).
+User-facing billing docs: [`docs/platform/billing.mdx`](../platform/billing.mdx). Behaviour test cases: `test/billing-*.md`.

As per coding guidelines: "Never link outside /docs from docs markdown files; reference external paths as inline code instead".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/contributing/billing.md` at line 93, In docs/contributing/billing.md the
markdown link [`test/billing-*.md`](../../test/) points outside /docs and must
be converted to inline code; replace the link token
[`test/billing-*.md`](../../test/) with an inline code span such as
`../../test/` or ``test/billing-*.md`` so the external /test path is referenced
as code rather than a clickable link.

docs/platform/billing.mdx-18-29 (1)

18-29: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

This page documents multi-seat billing that v1 does not ship.

PR #705 is explicitly single-seat only, but this copy promises per-seat subscriptions, automatic seat scaling, invite/remove proration, and pooled credits that grow with headcount. That will set the wrong expectation for customers and support. Please rewrite these sections to describe the shipped quantity=1 behavior and call multi-seat/proration out as a future limitation instead.

Also applies to: 91-100, 150-169

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/platform/billing.mdx` around lines 18 - 29, The billing doc currently
describes multi-seat features (per-seat pricing, pooled credits, automatic seat
scaling, proration) but PR `#705` ships single-seat only; update the copy to state
the shipped behavior is quantity=`1` (solo single-seat subscriptions), remove or
reword definitive language such as "Pro and Team are both seat-based." and "AI
credit pools at the **organization** level" to instead note per-seat/pooled
credits, proration, and auto-scaling are future features/limitations, and add
one clear sentence calling out that multi-seat billing, invite/remove proration,
and pooled credits are not available in v1 and will be addressed in a future
release; ensure references to annual discounts remain accurate but do not imply
multi-seat proration is supported.

editor/app/(site)/organizations/[organization_name]/settings/billing/upgrade/page.tsx-17-25 (1)

17-25: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce org ownership before showing the billing upgrade flow.

This only checks that the user is signed in, then loads any organization by slug. If the organization row is readable to non-owners, another logged-in user can reach another org’s billing upgrade page and potentially trigger billing actions for it. Please scope this query by owner_id = auth.user.id or perform an explicit membership/role check before rendering UpgradeView.

Suggested guard

-  const { data: org } = await client
+  const { data: org } = await client
     .from("organization")
-    .select("id, name")
+    .select("id, name, owner_id")
     .eq("name", organization_name)
     .single();
 
   if (!org) return notFound();
+  if (org.owner_id !== auth.user.id) return notFound();
 
   return <UpgradeView orgId={org.id} orgName={org.name} />;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@editor/app/`(site)/organizations/[organization_name]/settings/billing/upgrade/page.tsx
around lines 17 - 25, The org fetch currently only filters by slug
(organization_name) before rendering UpgradeView; tighten this by ensuring the
requesting user is the org owner or has appropriate billing role: modify the
query built from client.from("organization").select(...) to also filter by
owner_id = auth.user.id (or the equivalent current user id variable) or,
alternatively, perform an explicit membership/role check against your membership
table (e.g., organization_membership where user_id = auth.user.id and role IN
('owner','billing')) before returning <UpgradeView orgId={org.id}
orgName={org.name} />; ensure you reference the same symbols (organization_name,
client.from("organization").select, UpgradeView) so only authorized users can
reach the billing flow.

editor/types/types.ts-118-125 (1)

118-125: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

plan should not be required on a type you describe as conditional.

The docblock states this field is only present on responses that went through the workspace API path, but OrganizationWithAvatar makes it mandatory everywhere. This creates a type-soundness issue where organization.plan appears safe to access but may be undefined at runtime. Either split this into a billing-specific response type or make plan optional and narrow it where the API guarantees it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/types/types.ts` around lines 118 - 125, The OrganizationWithAvatar
type marks the conditional billing field plan (PlanTier) as required even though
the docblock says it only exists on certain API responses; change this by making
plan optional (plan?: PlanTier | undefined) or by creating a separate
billing-specific type (e.g., OrganizationWithAvatarAndPlan extends
OrganizationWithAvatar { plan: PlanTier }) and use that narrower type where the
workspace API guarantees the field; update any call sites that assume
organization.plan to either narrow the type (type guard or runtime check) or use
the billing-specific response type (refer to OrganizationWithAvatar and PlanTier
to locate the declaration and adjust consumers accordingly).

editor/app/(site)/organizations/[organization_name]/settings/billing/_view.tsx-505-509 (1)

505-509: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Gate the sandbox disclaimer behind an explicit test-mode signal.

This note renders for every organization, so live customers will be told their card is never billed. That is materially false outside billing test mode.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@editor/app/`(site)/organizations/[organization_name]/settings/billing/_view.tsx
around lines 505 - 509, The sandbox disclaimer paragraph (the <p> after
Separator in _view.tsx) must be rendered only when the organization is in
billing test mode; wrap that paragraph (or the whole block) in a conditional
that checks an explicit test-mode signal (e.g., organization.billingTestMode ||
organization.isBillingTestMode || a prop/hook like isBillingTestMode) so live
organizations don't see the message, and handle possible undefined
organization/billing fields safely (optional chaining or null checks); update
any callers/props or fetch logic to supply the test-mode flag if missing and
add/update a small test to assert the disclaimer only appears when that flag is
true.

editor/lib/billing/index.ts-274-308 (1)

274-308: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

resolveOrCreateStripeCustomer() can leak orphan Stripe customers.

The Stripe customer is created before the attach path is known to have succeeded. If the DB call fails, or two requests race past the initial cache miss, one of those external customers is dropped on the floor and every retry can mint another one.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/lib/billing/index.ts` around lines 274 - 308,
resolveOrCreateStripeCustomer currently creates a Stripe customer before
ensuring the DB attach succeeded, which can leak orphan customers and race; fix
by re-checking getCustomerId(org_id) immediately after the initial cache miss
and before calling stripe.customers.create to avoid races, and if you still
create a Stripe customer then call
service_role.workspace.rpc("fn_billing_attach_stripe_customer", ...) but ensure
that on any attach failure you call stripe.customers.del(created.id) (or
schedule deletion) to clean up the newly created customer; update
resolveOrCreateStripeCustomer to perform the double-check-before-create and the
cleanup-on-attach-failure around stripe.customers.create and the
fn_billing_attach_stripe_customer RPC.

editor/app/(site)/organizations/[organization_name]/settings/billing/_view.tsx-232-257 (1)

232-257: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Show the error state before the skeleton gate.

When getBillingSummary() fails on first load, state stays null and err is set, but if (loading || !state) still returns the skeleton. That makes the retry UI unreachable and the page looks stuck loading forever.

💡 Minimal fix

-  if (loading || !state) {
+  if (loading) {
     return (
       <main className="container mx-auto py-10 max-w-4xl">
         <header className="mb-8">
           <h1 className="text-3xl font-bold">Billing</h1>
         </header>
@@
-  if (err) {
+  if (err && !state) {
     return (
       <main className="container mx-auto py-10 max-w-4xl">
         <h1 className="text-3xl font-bold mb-4">Billing</h1>
         <p className="text-destructive">Failed to load billing: {err}</p>
         <Button onClick={refresh} className="mt-4" variant="outline">

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@editor/app/`(site)/organizations/[organization_name]/settings/billing/_view.tsx
around lines 232 - 257, The error UI is currently unreachable because the
skeleton gate (if (loading || !state)) runs before checking err; change the
rendering order so the err check runs before the skeleton check (i.e., render
the error return when err is truthy) or modify the skeleton condition to exclude
error (e.g., if (loading || (!state && !err))). Update the component logic
around loading, state, and err (references: getBillingSummary(), refresh,
loading, state, err) so the Retry button and error message render when
getBillingSummary() fails on first load.

supabase/schemas/grida_billing.sql-612-643 (1)

612-643: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't collapse first-invoice failures into past_due.

This handler always writes past_due, but the rest of the PR distinguishes first-payment failures as incomplete / incomplete_expired. Stripe retains newly-created subscriptions in incomplete status initially (for ~23 hours) when the first invoice payment fails, transitioning to incomplete_expired if unpaid—it never uses past_due for first-payment failures. The current code unconditionally overwrites these states, causing the final status to become webhook-order dependent and the UI can fall back to the renewal-failure path for a brand-new subscription.

Mitigation

-      UPDATE grida_billing.subscription
-         SET status = 'past_due', updated_at = now()
-       WHERE stripe_subscription_id = v_sub_id;
+      UPDATE grida_billing.subscription
+         SET status = CASE
+               WHEN status IN ('incomplete', 'incomplete_expired') THEN status
+               ELSE 'past_due'
+             END,
+             updated_at = now()
+       WHERE stripe_subscription_id = v_sub_id;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/schemas/grida_billing.sql` around lines 612 - 643, The handler
currently unconditionally sets subscription.status to 'past_due' for
invoice.payment_failed; instead detect first-invoice failures using
v_attempt_count (and/or the current subscription row) and preserve or set
'incomplete'/'incomplete_expired' as appropriate: SELECT status INTO
v_current_status FROM grida_billing.subscription WHERE stripe_subscription_id =
v_sub_id (reuse v_org_id lookup), then if v_current_status = 'incomplete' leave
it unchanged (or if v_attempt_count IS NULL or = 0 set status to 'incomplete'),
otherwise set status = 'past_due'; also write the same computed status into the
INSERT to grida_billing.audit and keep v_handler logic (v_handler :=
'invoice_payment_failed' / 'invoice_payment_failed_skipped') consistent. Ensure
you reference v_attempt_count, v_sub_id, v_org_id, v_current_status and the
grida_billing.subscription update/insert statements when making the change.

supabase/migrations/20260506132900_grida_billing.sql-794-805 (1)

794-805: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Use a safe search_path in the SECURITY DEFINER wrappers.

These public wrappers run as SECURITY DEFINER with search_path = grida_billing, public, pg_temp. That is the unsafe pattern the repo rules explicitly ban; pg_catalog should be first, and pg_temp should not be in the definer path. The same fix applies to the other public.fn_billing_* wrappers below.

Suggested fix

-SET search_path = grida_billing, public, pg_temp
+SET search_path = pg_catalog, public

As per coding guidelines, "For SECURITY DEFINER functions, set a safe search_path with SET search_path = pg_catalog, public".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/migrations/20260506132900_grida_billing.sql` around lines 794 - 805,
The SECURITY DEFINER wrapper public.fn_billing_apply_stripe_event (and the other
public.fn_billing_* wrappers) uses an unsafe search_path; change the SET
search_path clause to a safe value by setting "SET search_path = pg_catalog,
public" (remove pg_temp and ensure pg_catalog is first) so the wrapper still
calls grida_billing.fn_apply_stripe_event but runs with a safe definer
search_path.

editor/app/(site)/organizations/[organization_name]/settings/billing/_actions.ts-318-326 (1)

318-326: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Block plan changes for non-healthy subscriptions.

getActivePaidSubscription() returns any non-canceled paid row, so this path still opens subscription_update_confirm for past_due, unpaid, incomplete, and similar states. That contradicts the PR requirement to disable plan changes until payment issues are resolved.

Suggested fix

   const sub = await getActivePaidSubscription(org_id);
   if (!sub) {
     throw new BillingError(
       "No active paid subscription to change. Upgrade first.",
       "not_subscribed",
       400,
       "billing/upgrade"
     );
   }
+  if (sub.status !== "active" && sub.status !== "trialing") {
+    throw new BillingError(
+      "Resolve the current billing issue before changing plans.",
+      "plan_change_blocked",
+      409,
+      "billing"
+    );
+  }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@editor/app/`(site)/organizations/[organization_name]/settings/billing/_actions.ts
around lines 318 - 326, getActivePaidSubscription currently returns any
non-canceled paid row, so before allowing plan-change flow (the branch that
would open subscription_update_confirm) validate the subscription health by
checking the returned sub.status (from getActivePaidSubscription) against
allowed healthy states (e.g., "active") or reject known bad states
("past_due","unpaid","incomplete", etc.); if the status is not healthy, throw
the existing BillingError (the same class used here) with a payment-issue
message and appropriate code so plan changes are blocked until payment is
resolved. Ensure you update the branch that currently proceeds after const sub =
await getActivePaidSubscription(org_id) to perform this status check and throw
early when unhealthy.

supabase/migrations/20260506132900_grida_billing.sql-502-513 (1)

502-513: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail closed when a Stripe price is unmapped.

Falling back to "pro" here will silently mis-project any unknown or newly added price as Pro. That writes the wrong plan into the local source of truth and can under/over-entitle the org until someone fixes it manually.

Suggested fix

-    v_plan := 'pro';
+    v_plan := NULL;
     IF v_price_id IS NOT NULL THEN
       SELECT CASE
                WHEN id IN ('plan.team', 'plan.team.annual') THEN 'team'
                WHEN id IN ('plan.pro',  'plan.pro.annual')  THEN 'pro'
-               ELSE 'pro'
+               ELSE NULL
              END
         INTO v_plan
         FROM grida_billing.product_catalogue
        WHERE stripe_price_id = v_price_id;
-      v_plan := coalesce(v_plan, 'pro');
     END IF;
+    IF v_plan IS NULL THEN
+      RAISE EXCEPTION 'subscription % has unknown stripe price %', v_sub_id, v_price_id;
+    END IF;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/migrations/20260506132900_grida_billing.sql` around lines 502 - 513,
The current logic in the block around v_plan, v_price_id and the
grida_billing.product_catalogue lookup silently falls back to 'pro' for unmapped
stripe_price_id values; change it to fail-fast: after attempting the SELECT ...
INTO v_plan from product_catalogue (matching on stripe_price_id), check whether
a row was FOUND or whether v_plan is NULL and if so RAISE EXCEPTION (including
the v_price_id for context) instead of coalescing to 'pro'; this keeps the
incorrect default from being written and surfaces unmapped/new Stripe prices for
explicit handling.

supabase/migrations/20260506132900_grida_billing.sql-353-370 (1)

353-370: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t return attached=true when no account row was updated.

If provisioning failed or p_org_id is invalid, SELECT ... FOR UPDATE finds no row, the UPDATE affects 0 rows, and this function still reports success and writes an audit entry. Callers will believe the customer is cached even though grida_billing.account is unchanged.

Suggested fix

   SELECT acc.stripe_customer_id INTO v_existing
     FROM grida_billing.account acc
    WHERE acc.organization_id = p_org_id
    FOR UPDATE;
+
+  IF NOT FOUND THEN
+    RAISE EXCEPTION 'billing account not provisioned for organization %', p_org_id;
+  END IF;

   IF v_existing IS NULL THEN
     UPDATE grida_billing.account
        SET stripe_customer_id = p_stripe_customer_id,
            updated_at         = now()
      WHERE organization_id = p_org_id;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/migrations/20260506132900_grida_billing.sql` around lines 353 - 370,
The function currently sets v_attached = true and writes an audit even when no
account row was actually updated; after the UPDATE on grida_billing.account (the
statement that uses WHERE organization_id = p_org_id) capture whether the UPDATE
affected a row (use IF FOUND or GET DIAGNOSTICS ROW_COUNT into a local var) and
only set v_attached := true and INSERT into grida_billing.audit when the UPDATE
affected 1 row; if no row was updated, leave v_attached false (or return an
error) and do not write the audit. Ensure you reference and check v_existing,
p_org_id and p_stripe_customer_id around that UPDATE/INSERT block.

supabase/migrations/20260506132900_grida_billing.sql-442-446 (1)

442-446: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Lock the stripe_event row before the replay check.

Two concurrent deliveries of the same event can both read processed_at IS NULL here and execute the projector body, so idempotency only holds after the first commit. That leaves duplicate audit rows and makes the webhook ingest contract racey.

Suggested fix

-  SELECT * INTO v_existing FROM grida_billing.stripe_event WHERE id = p_event_id;
+  SELECT *
+    INTO v_existing
+    FROM grida_billing.stripe_event
+   WHERE id = p_event_id
+   FOR UPDATE;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/migrations/20260506132900_grida_billing.sql` around lines 442 - 446,
The race comes from reading grida_billing.stripe_event into v_existing without
locking, so two concurrent deliveries can see processed_at IS NULL and both
proceed; change the fetch to acquire a row lock (SELECT ... FOR UPDATE) on
grida_billing.stripe_event for p_event_id before checking
v_existing.processed_at so the second transaction will wait and observe the
committed processed_at; update the SELECT INTO that populates v_existing (and
any related reads) to use FOR UPDATE within the same function/transaction that
handles the projector.

editor/.env.test-11-15 (1)

11-15: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Precedence comment is incorrect

The comment in .env.test states that process.env (from your shell) has the lowest priority (#1), overridden by .env.test.local (#3). However, vitest.config.ts explicitly states: "Existing process.env always wins."

The loadEnvFile() function only sets variables that don't already exist in process.env (if (!(key in process.env))), making shell-injected env vars have the highest priority, not the lowest.

Update the comment to reflect actual behavior: existing process.env values are never overridden by either .env.test or .env.test.local.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/.env.test` around lines 11 - 15, Update the inaccurate precedence
comment in .env.test to reflect actual behavior: existing process.env values are
never overridden by env files. Mention that vitest.config.ts's loadEnvFile()
(which checks if (!(key in process.env))) gives shell-injected variables highest
priority, and that .env.test and .env.test.local only set variables when not
already present (with .env.test.local remaining for per-developer secrets but
still not overriding process.env).

editor/app/(site)/organizations/[organization_name]/settings/_shell.tsx-37-43 (1)

37-43: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

"Billing" and "Upgrade plan" are both highlighted simultaneously on the upgrade page.

When pathname is /organizations/{org}/settings/billing/upgrade, the startsWith check at line 71 marks "Billing" as active (because /billing/upgrade starts with /billing/), and "Upgrade plan" is also active via the exact-match. Two items light up at once.

Fix by using exact matching for "Billing" only when sub-items are visible, or by excluding child paths explicitly:

🐛 Proposed fix

  const isActive =
-   pathname === c.href || pathname?.startsWith(`${c.href}/`);
+   pathname === c.href ||
+   (pathname?.startsWith(`${c.href}/`) &&
+     !categories.some((other) => other !== c && other.href.startsWith(`${c.href}/`) && pathname?.startsWith(other.href)));

Or the simpler, explicit approach — change the "Billing" entry to use exact match only since "Upgrade plan" already covers the sub-path:

  {categories.map((c) => {
    const isActive =
-     pathname === c.href || pathname?.startsWith(`${c.href}/`);
+     c.href === `${settingsBase}/billing`
+       ? pathname === c.href
+       : pathname === c.href || pathname?.startsWith(`${c.href}/`);

Also applies to: 69-79

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/app/`(site)/organizations/[organization_name]/settings/_shell.tsx
around lines 37 - 43, The "Billing" menu item in the categories array is being
marked active alongside its child "Upgrade plan" because the active check uses
startsWith on pathname; update the active resolution so parent items like the
Billing entry (in the categories const in _shell.tsx) use exact matching when a
child route exists: either change the Billing entry to be matched exactly
instead of via startsWith, or modify the active detection logic to prefer exact
path equality for parent items when any explicit child path (e.g.,
`${settingsBase}/billing/upgrade`) is present so only the intended item is
highlighted.

editor/vitest.config.ts-7-8 (1)

7-8: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Precedence comment has the order backwards.

The code gives process.env (shell) the highest priority (if (!(key in process.env))), so the true order is shell > .env.test.local > .env.test. The comment currently lists shell as the lowest priority, which will mislead developers who expect their .env.test.local values to override a shell-exported variable.

📝 Suggested correction

-// Precedence (later loses):
-// .env.test.local > .env.test > shell. Existing process.env always wins.
+// Precedence (highest wins):
+// shell > .env.test.local > .env.test. Existing process.env always wins.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/vitest.config.ts` around lines 7 - 8, The precedence comment is
backwards: update the explanatory comment that starts with "without `set -a; .
./.env.test.local` ceremony. Precedence (later loses):" so it correctly
describes that existing process.env (shell) currently wins due to the `if (!(key
in process.env))` check; change the order to "shell > .env.test.local >
.env.test" (or reword to say "existing process.env takes precedence over
.env.test.local which takes precedence over .env.test") so the comment matches
the actual behavior implemented by that conditional.

docs/platform/billing.mdx-1-6 (1)

1-6: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add keywords to the frontmatter.

This page is under docs/platform/** and already has title/description, so it is missing the remaining required SEO field. As per coding guidelines, docs/{wg,reference,editor,forms,platform,with-figma,design,math}/**/*.{md,mdx}: Add SEO frontmatter with title, description, and keywords when adding or meaningfully editing actively maintained doc pages.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/platform/billing.mdx` around lines 1 - 6, The frontmatter in the Billing
doc is missing the required SEO `keywords` field; update the YAML frontmatter
that currently contains `title:`, `description:` (as shown in the file header)
to include a `keywords:` entry with a comma-separated list of relevant SEO terms
(e.g., "billing, pricing, AI credits, plans") so the page complies with the docs
frontmatter rule for docs/platform pages.

docs/platform/billing.mdx-193-194 (1)

193-194: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep docs links inside /docs.

/support/refund-policy is an internal route outside the docs tree, so this link breaks the repository rule for markdown docs. Point to the docs version of that policy instead, or reference the non-doc path as inline code. As per coding guidelines, docs/**/*.{md,mdx}: Never link outside /docs from docs markdown files; reference external paths as inline code instead.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/platform/billing.mdx` around lines 193 - 194, The link in the "Can I get
a refund?" section points to an internal non-doc route (/support/refund-policy)
which violates the docs rule; update the link to point to the docs-hosted policy
path (e.g., the docs equivalent under /docs/... or the canonical docs route for
the refund policy) or, if you must reference the non-doc path, replace the
markdown link with inline code notation for /support/refund-policy so it no
longer creates an external docs link.

editor/app/(site)/organizations/[organization_name]/settings/billing/_actions.ts-596-603 (1)

596-603: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

The audit cursor can skip rows at page boundaries.

The result set is ordered by (created_at DESC, id DESC), but the next page only filters on created_at < cursor. Any rows that share the boundary timestamp become unreachable. Use a composite cursor (created_at, id) or switch the cursor to the monotonic id.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@editor/app/`(site)/organizations/[organization_name]/settings/billing/_actions.ts
around lines 596 - 603, The current pagination uses only created_at as the
cursor in the query built in the block that sets `if (cursor) q =
q.lt("created_at", cursor);` and computes `next_cursor` from `rows[rows.length -
1].created_at`, which can skip rows that share the same timestamp; change the
cursor to a composite cursor of (created_at, id) or use the monotonic `id` only:
update the query building (the `q` construction) to accept a composite cursor
and apply a predicate equivalent to (created_at < cursor.created_at OR
(created_at = cursor.created_at AND id < cursor.id)), update how `cursor` is
passed/parsed to include both `created_at` and `id`, and set `next_cursor` to
include both values (e.g., the last row's created_at and id) so page boundaries
are deterministic and no rows are skipped; alternatively, replace the created_at
filter entirely with `id < cursorId` and compute `next_cursor` from the last
row's id if you prefer a monotonic id cursor.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

editor/lib/billing/__tests__/e2e/fixtures/org.ts (1)

71-75: 💤 Low value

Use Stripe error code property instead of message regex matching.

The current catch blocks use /No such subscription|resource_missing/i.test(msg) to detect "already gone" errors. Stripe errors expose a structured code property (e.g., 'resource_missing') and the stripe.errors.StripeError type — matching against the freeform message is fragile and could unintentionally match other errors containing those words. The official Stripe pattern is to check err instanceof stripe.errors.StripeError && err.code === "resource_missing".

♻️ Proposed changes

        const canceled = await stripe.subscriptions
          .cancel(sub.id)
          .catch((err: unknown) => {
            const msg = err instanceof Error ? err.message : String(err);
            if (/No such subscription|resource_missing/i.test(msg)) return null;
            throw err;
          });

Replace with:

        const canceled = await stripe.subscriptions
          .cancel(sub.id)
          .catch((err: unknown) => {
            if (
              err instanceof stripe.errors.StripeError &&
              err.code === "resource_missing"
            ) {
              return null;
            }
            throw err;
          });

And at lines 91–98:

    try {
      await stripe.customers.del(customerId);
    } catch (err) {
      const msg = err instanceof Error ? err.message : String(err);
      // "Already deleted" is fine — fall through to the local org delete.
      // Anything else is a real teardown failure and must not be silently
      // logged — orphan customers compound across runs and eventually trip
      // the test sandbox limits.
-     if (!/No such customer|resource_missing/i.test(msg)) {
-       throw new Error(`[e2e/org] customer delete failed: ${msg}`);
+     const isMissing =
+       err instanceof stripe.errors.StripeError &&
+       err.code === "resource_missing";
+     if (!isMissing) {
+       const msg = err instanceof Error ? err.message : String(err);
+       throw new Error(`[e2e/org] customer delete failed: ${msg}`);
       }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@editor/lib/billing/__tests__/e2e/fixtures/org.ts` around lines 71 - 75,
Replace fragile regex message-matching in the promise rejection handler (the
.catch((err: unknown) => { ... }) block in the org fixture) with a structured
Stripe error check: detect Stripe errors using err instanceof
stripe.errors.StripeError and then check err.code for expected values (e.g.,
'resource_missing' and/or 'no_such_subscription') and return null for those
codes; otherwise rethrow the error. Ensure you reference the same catch block
and use the stripe.errors.StripeError type and err.code comparisons instead of
testing the freeform message.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes